Skip to content

Add display brightness control #83

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

weberval
Copy link

@weberval weberval commented Jun 21, 2025

Summary by Sourcery

Introduce brightness control by adding a Brightness enum and integrating it into the protocol header, payload buffer, and CLI configuration, along with related tests and minor documentation fixes.

New Features:

  • Add Brightness enum with four levels and conversions to/from u8
  • Extend protocol header and payload buffer to include brightness control
  • Enable CLI config and payload generation to set display brightness

Enhancements:

  • Adjust MAGIC constant length from 6 to 5 to accommodate brightness field
  • Add set_brightness method and default initialization for PayloadBuffer

Documentation:

  • Fix typos in documentation comments (e.g., "around", "through", "consists", buffer instantiation)

Tests:

  • Add unit test for brightness to u8 conversion

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @weberval - I've reviewed your changes - here's some feedback:

  • The brightness_to_u8 test calls Brightness::from(i.0) incorrectly – it should directly use u8::from(i.0) since you only implemented From<Brightness> for u8.
  • Consider providing a builder-style .brightness() setter or allowing brightness to be modified on an existing PayloadBuffer for consistency with other style setters instead of only via the constructor.
  • Changing the header size by removing a magic byte and inserting brightness breaks protocol compatibility; ensure you document this change clearly and bump the protocol version accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `brightness_to_u8` test calls `Brightness::from(i.0)` incorrectly – it should directly use `u8::from(i.0)` since you only implemented `From<Brightness> for u8`.
- Consider providing a builder-style `.brightness()` setter or allowing brightness to be modified on an existing `PayloadBuffer` for consistency with other style setters instead of only via the constructor.
- Changing the header size by removing a magic byte and inserting `brightness` breaks protocol compatibility; ensure you document this change clearly and bump the protocol version accordingly.

## Individual Comments

### Comment 1
<location> `src/protocol.rs:536` </location>
<code_context>
+            (Brightness::OneQuarter, 0x30),
+        ];
+
+        for i in VALID_BRIGHTNESS_VALUES {
+            assert_eq!(u8::from(Brightness::from(i.0)), i.1);
+        }
</code_context>

<issue_to_address>
Redundant use of Brightness::from in test.

You can simplify `u8::from(Brightness::from(i.0))` to `u8::from(i.0)` since `i.0` is already a Brightness.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@weberval
Copy link
Author

I'm still waiting for review. Is there any chance this happens in the next month?

@mgjm
Copy link
Collaborator

mgjm commented Jul 10, 2025

Before I start with the review, can you please point me to the place where those brightness values are used in the firmware.

I could only find a brightness value controlled by button presses or a new BLE characteristic. Or is this only implemented in the OEM firmware?

@mgjm
Copy link
Collaborator

mgjm commented Jul 15, 2025

@weberval I don't want to put you under pressure or anything, I just want to mention you explicitly in case you weren't notified about the previous message.

@weberval
Copy link
Author

It seems it is not (yet) implemented in the OSS firmware.

Instead, it seems like the "magic" header is longer than required and the brightness value is the last byte of that header. (see firmware vs my protocol implementation)

I have not found any validation of the magic header wang\0\0 in the OSS firmware, so I think it should be fine already implementing this feature in the client library and opening an issue on the OSS firmware repository.

@mgjm
Copy link
Collaborator

mgjm commented Jul 15, 2025

@weberval Do you have a link to a fork of the firmware with the brightness already implemented? Is this implemented in any other software to talk to the badges (e.g. Android App, Go implementation, etc.)?

If this is not implemented anywhere else, how / why did you pick the values 0x00, 0x10, 0x20 and 0x30? How are the other bits defined (0x01 ..= 0x0F and 0x40 ..= 0xFF)? (Must be zero, reserved for future use, etc.)

@weberval
Copy link
Author

weberval commented Jul 15, 2025

I used the values from the python project. I don't know what padding is applied to the value in the proprietary firmware or what the other bits could be used for, but those values seem to work.

@mgjm
Copy link
Collaborator

mgjm commented Jul 15, 2025

Ok, awesome. So it is from a commit from 2019 (fossasia/led-name-badge-ls32@6640ee3). Therefore I would assume that this is implemented in the proprietary firmware from the manufacturer.

Then lets look at the actual code...

Copy link
Collaborator

@mgjm mgjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I have added a few comments. And thanks for fixing a lot of my typos 👍

@mgjm
Copy link
Collaborator

mgjm commented Jul 15, 2025

Note

I just updated the dependencies and fixed the new clippy lints. You can pull these changes into your PR.

@weberval weberval force-pushed the feat/brightnesscontrol branch from 4894bde to a9b38fb Compare July 16, 2025 17:56
@weberval weberval requested a review from mgjm July 16, 2025 18:07
@weberval
Copy link
Author

@sourcery-ai dismiss

@weberval
Copy link
Author

@sourcery-ai review

Copy link

@SourceryAI SourceryAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @weberval - I've reviewed your changes and they look great!

Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments

### Comment 1
<location> `src/protocol.rs:214` </location>
<code_context>
+    }
+}
+
+impl TryFrom<u8> for Brightness {
+    type Error = TryFromIntError;
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        Ok(match value {
+            0x00 => Self::Full,
+            0x10 => Self::ThreeQuarters,
+            0x20 => Self::Half,
+            0x30 => Self::OneQuarter,
+            _ => return Err(u8::try_from(-1).unwrap_err()),
+        })
+    }
</code_context>

<issue_to_address>
Using u8::try_from(-1) to generate an error is indirect and may be confusing.

Use a more explicit error, like core::num::TryFromIntError::default() or a custom error type, to clarify intent and avoid depending on u8::try_from(-1).
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
+impl TryFrom<u8> for Brightness {
+    type Error = TryFromIntError;
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        Ok(match value {
+            0x00 => Self::Full,
+            0x10 => Self::ThreeQuarters,
+            0x20 => Self::Half,
+            0x30 => Self::OneQuarter,
+            _ => return Err(u8::try_from(-1).unwrap_err()),
+        })
+    }
=======
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub struct BrightnessParseError;
+
+impl core::fmt::Display for BrightnessParseError {
+    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+        write!(f, "invalid value for Brightness")
+    }
+}
+
+#[cfg(feature = "std")]
+impl std::error::Error for BrightnessParseError {}
+
+impl TryFrom<u8> for Brightness {
+    type Error = BrightnessParseError;
+
+    fn try_from(value: u8) -> Result<Self, Self::Error> {
+        match value {
+            0x00 => Ok(Self::Full),
+            0x10 => Ok(Self::ThreeQuarters),
+            0x20 => Ok(Self::Half),
+            0x30 => Ok(Self::OneQuarter),
+            _ => Err(BrightnessParseError),
+        }
+    }
>>>>>>> REPLACE

</suggested_fix>

### Comment 2
<location> `src/protocol.rs:342` </location>
<code_context>
+    }
+
     /// Return the current number of messages
     pub fn num_messages(&mut self) -> usize {
         self.num_messages as usize
</code_context>

<issue_to_address>
The num_messages method takes &mut self but does not mutate state.

Changing the receiver to &self would improve flexibility since no mutation occurs.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
    /// Return the current number of messages
    pub fn num_messages(&mut self) -> usize {
        self.num_messages as usize
        &self.data
    }
=======
    /// Return the current number of messages
    pub fn num_messages(&self) -> usize {
        self.num_messages as usize
    }
>>>>>>> REPLACE

</suggested_fix>

Hi @weberval! 👋

Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀

Install the sourcery-ai bot to get automatic code reviews on every pull request ✨

Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 406 to 407
&self.data
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: The num_messages method takes &mut self but does not mutate state.

Changing the receiver to &self would improve flexibility since no mutation occurs.

Suggested change
&self.data
}
/// Return the current number of messages
pub fn num_messages(&self) -> usize {
self.num_messages as usize
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this change set, but should be fixed 😅

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @weberval - I've reviewed your changes - here's some feedback:

  • The TryFrom implementation for Brightness uses a workaround (u8::try_from(-1).unwrap_err()) to generate errors—consider defining a dedicated error type or using a more idiomatic approach for invalid values.
  • Changing the protocol header (MAGIC length changed and a brightness byte added) is a breaking change—make sure the version bump follows semver (major version) or include upgrade notes.
  • Consider adding unit tests for invalid brightness inputs to verify that TryFrom correctly returns Err for out-of-range values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The TryFrom<u8> implementation for Brightness uses a workaround (`u8::try_from(-1).unwrap_err()`) to generate errors—consider defining a dedicated error type or using a more idiomatic approach for invalid values.
- Changing the protocol header (MAGIC length changed and a brightness byte added) is a breaking change—make sure the version bump follows semver (major version) or include upgrade notes.
- Consider adding unit tests for invalid brightness inputs to verify that TryFrom<u8> correctly returns Err for out-of-range values.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@fossasia fossasia deleted a comment from sourcery-ai bot Jul 16, 2025
Copy link
Collaborator

@mgjm mgjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few tiny points left. But I thought a bit about the current representation in the config file:

brightness = "full"
brightness = "three-quarters"
brightness = "half"
brightness = "one-quarter"

Feels a bit weird to write the names out. Alternative representations:

  • Percentage numbers like in the python implementation:
    brightness = 100
    brightness = 75
    brightness = 50
    brightness = 25
  • Percentages but with a percent sign (needs to be a string):
    brightness = "100%"
    brightness = "75%"
    brightness = "50%"
    brightness = "25%"
  • Consecutively numbered brightness levels:
    brightness = 4
    brightness = 3
    brightness = 2
    brightness = 1
  • Floating point numbers with 1 as the max:
    brightness = 1
    brightness = 0.75
    brightness = 0.5
    brightness = 0.25

I think I like the float variant the most since it allows for future extension if the OSS firmware would add additional brightness levels (The percent based variants would need floats to represent 8 brightness levels 12.5%, etc.). But I am open to other opinions.

@weberval
Copy link
Author

I haven't thought about his at all, but it makes sense. Option four seems to be the most intuitive, so I'm in favour of that, too.

* Floating point numbers with `1` as the max:
  ```toml
  brightness = 1
  brightness = 0.75
  brightness = 0.5
  brightness = 0.25
  ```

@weberval weberval requested a review from mgjm July 18, 2025 20:04
@weberval weberval changed the title feat: add brightnesscontrol feat: add brightnesscontrol @sourcery-ai Jul 20, 2025
@sourcery-ai sourcery-ai bot changed the title feat: add brightnesscontrol @sourcery-ai Add display brightness control Jul 20, 2025
@weberval
Copy link
Author

weberval commented Jul 29, 2025

@mgjm Have you had the time to have a look at my changes?
Any additional input?

@mariobehling
Copy link
Member

@weberval @mgjm Nice work. Hope this can be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants